test: Obliterate & revive integration tests (fixes #8071) #9266
Conversation
* main: lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
There was a problem hiding this comment.
Great, tests that pass and in reasonable time - few minor/optional comments, overall lgtm
I have some sympathy for the old scenarios of throwing pretty wild scenarios at it, I think it did provide some insights in the past. Also I have vague memories of the benchmarks being useful. Not arguing against anything, just some nice passing words for those :D
| addrExp := regexp.MustCompile(`GUI and API listening on ([^\s]+)`) | ||
| myIDExp := regexp.MustCompile(`My ID: ([^\s]+)`) | ||
| tcpAddrExp := regexp.MustCompile(`TCP listener \((.+)\) starting`) | ||
| go func() { |
There was a problem hiding this comment.
This goroutine never stops as far as I see. Not impactful, but less noise in goroutine stacks is always nice, so I think would be nice to limit it's lifetime to the command.
There was a problem hiding this comment.
It should exit when sc.Read fails, which will happen when the io.Reader returns an error (io.EOF or similar, when the process exits), so this should only live for the lifetime of the process.
There was a problem hiding this comment.
I'd like it to be scoped to the lifetime of a tests though - I don't see that reader (from io.Pipe in startInstance) being closed at the end of a test. What am I missing?
There was a problem hiding this comment.
Refactoring to happen, take this all with a grain of salt for now.
lib/rc/rc.go
Outdated
There was a problem hiding this comment.
There's not much left in this package - it's now just an API helper and most of the process handling now lives in ./test. My gut feeling is to remove this package and also move the remaining code to test.
There was a problem hiding this comment.
Hi, longtime Go programmer and SyncThing user here.
I have been reading the source code and pull requests recently, and humbly offer some comments for your consideration. Please excuse my presumptuousness, and feel free to entirely ignore me if you don't find the comments helpful.
Cheers, and thanks for working on SyncThing.
| addrExp := regexp.MustCompile(`GUI and API listening on ([^\s]+)`) | ||
| myIDExp := regexp.MustCompile(`My ID: ([^\s]+)`) | ||
| tcpAddrExp := regexp.MustCompile(`TCP listener \((.+)\) starting`) | ||
| go func() { |
There was a problem hiding this comment.
It should exit when sc.Read fails, which will happen when the io.Reader returns an error (io.EOF or similar, when the process exits), so this should only live for the lifetime of the process.
| go func() { | ||
| for sc.Scan() { | ||
| line := sc.Text() | ||
| lr.log.WriteString(line + "\n") |
There was a problem hiding this comment.
nit:
could be lr.log.Write(sc.Bytes()) and lr.log.WriteByte('\n') to save allocations.
similarly below you could use the regexp FindSubmatch method instead of FindStringSubmatch and match on the line bytes, instead of a string
| }) | ||
|
|
||
| // Wait up to 30 seconds to get the device ID, which comes first. | ||
| select { |
There was a problem hiding this comment.
minor suggestion:
I'd be inclined to bundle all the concurrency related to this operation in the one function, so you have something like
func readSyncthingMetadata(r io.Reader) (*syncthingMetadata, error)
which handles all the timeouts etc. Then you don't need the struct with all the channels, the scanner goroutine can set the fields of a syncthingMetadata struct in place as it scans them, and then when it has populated all fields it can signal to the main goroutine that it's ready to be returned.
this would make error handling more natural as well, I think
| if m := myIDExp.FindStringSubmatch(line); len(m) == 2 { | ||
| id, err := protocol.DeviceIDFromString(m[1]) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
suggestion:
Add some context to these panic messages - the full line that has been matched would probably be good. Otherwise the parse error on its own might be bit mysterious.
I'd be inclined to actually return these rather than panicking, though. Since this goroutine will have a different stack to the one that is starting the instance, it might get painful to figure out which instance isn't starting properly.
test/sync_2dev_test.go
Outdated
| } | ||
| } | ||
|
|
||
| srcDone := make(chan struct{}) |
There was a problem hiding this comment.
suggest using sync.WaitGroup instead
test/sync_2dev_test.go
Outdated
| lastEventID := 0 | ||
| remoteCompletion := 0.0 | ||
| remoteIndexUpdated := false | ||
| loop: |
There was a problem hiding this comment.
If you're only using this label for break loop then you can lose the label and just return, since nothing happens after the loop.
* main: (24 commits) lib/ignore: Refactor out result type (syncthing#9343) build: Testing infra images for infra-* branches lib/versioner: Expand tildes in version directory (fixes syncthing#9241) (syncthing#9327) lib/scanner: Prevent sync-conflict for receive-only local modifications (syncthing#9323) gui, man, authors: Update docs, translations, and contributors Fix website security link in README.md (syncthing#9325) cmd/syncthing: Add CLI completion functionality (fixes syncthing#8616) (syncthing#9226) lib/api: Save session & CSRF tokens to database, add option to stay logged in (fixes syncthing#9151) (syncthing#9284) Update dependencies (syncthing#9321) gui: Always inform about loading data in Restore Versions modal (syncthing#9317) lib/build: Allow semver build in version regex (fixes syncthing#9267) (syncthing#9316) gui: Keep short deviceID length consistent + xrefs (fixes syncthing#9313) (syncthing#9314) build(deps): bump actions/download-artifact from 3 to 4 (syncthing#9294) build(deps): bump actions/upload-artifact from 3 to 4 (syncthing#9293) gui, man, authors: Update docs, translations, and contributors gui, lib/scanner: Improve scan progress indication (ref syncthing#8331) (syncthing#9308) lib/protocol: handle empty names in unixOwnershipEqual (fixes syncthing#9039) (syncthing#9306) gui, man, authors: Update docs, translations, and contributors etc/linux-desktop: use double dash for long options (syncthing#9301) lib/connections: Skip allocation in check for missing port (syncthing#9297) ...
|
@calmh I've patched this PR in my fork. Is it OK to make this change directly in this PR instead? I also resolved a small conflict (test/.gitignore) when I rebased main on this PR. |
|
For sure! It kind of got lost along the way |
|
@calmh My apologies if I polluted your PR. I was never presented an option to squash main's commits. I guess I should have $ gh repo clone syncthing/syncthing
$ cd syncthing
$ gh pr checkout 9266
$ git rebase main
CONFLICT (modify/delete): test/.gitignore deleted in 7b5cd3df5 (wip) and modified in HEAD. Version HEAD of test/.gitignore left in tree.
...
$ git rm test/.gitignore
rm 'test/.gitignore'
$ git rebase --continue
[detached HEAD 61a490459] wip
...
$ git rebase main
Current branch integrationtests is up to date.
$ git pull
From https://github.com/calmh/syncthing
* branch integrationtests -> FETCH_HEAD
Auto-merging .github/workflows/build-syncthing.yaml
CONFLICT (rename/delete): lib/rc/debug.go renamed to lib/automaxprocs/automaxprocs.go in HEAD, but deleted in 99dad07024f195e9f5b67bdac38c1ef2b86bd654.
CONFLICT (modify/delete): lib/automaxprocs/automaxprocs.go deleted in 99dad07024f195e9f5b67bdac38c1ef2b86bd654 and modified in HEAD. Version HEAD of lib/automaxprocs/automaxprocs.go left in tree.
Automatic merge failed; fix conflicts and then commit the result.
$ git add lib/automaxprocs/automaxprocs.go
$ git commit -am "Rebase branch 'main' into integrationtests"
$ git push
...
To https://github.com/calmh/syncthing.git
...
error: failed to push some refs to 'https://github.com/calmh/syncthing.git'
What did I do wrong? Do you want me to undo it with a |
|
Eh, yeah, no idea at the moment as I'm in the middle of a Friday night, but I'll take a look at some point :) |
|
@calmh Per https://stackoverflow.com/a/73951458, I used this scriptgit checkout --detach main git cherry-pick c35d3a1 git cherry-pick b4a54e4 git cherry-pick c05fbc6 git cherry-pick 14e8f10 git cherry-pick f67c77f git cherry-pick c57461a git cherry-pick 7a8fcd9 git cherry-pick a9ba810 git cherry-pick ed587cf git cherry-pick 7f017e6 git cherry-pick b0c48db git cherry-pick f9faed1 git cherry-pick cc80256 git cherry-pick f9a7c2b git cherry-pick ab0413b git cherry-pick 0cebda5 git cherry-pick cdf162d git cherry-pick 9e4c2f5 git cherry-pick 4c67bf7 git cherry-pick 57850a4 git cherry-pick e7f98f5 git cherry-pick 754db30 git cherry-pick de7569f git cherry-pick 643f5e9 git cherry-pick 5f569a7 git cherry-pick f7cdcab git branch -f integrationtests git push -fmain...rasa:syncthing:integrationtests with your changes on top of main. The only issue is the commit dates got reset. |
|
@calmh OK, so I fixed this PR's history, but in the process, I re-introduced the conflict with main. I will fix this once I make certain I won't screw things up again. |
|
@calm So I tried again to cleanly merge main into this PR with gh pr checkout 9266 --repo syncthing/syncthing
git merge --squash upstream/main
git rm test/.gitignore
git commit
git push, but the number of changed files again increased from 47 to 216. So I backed out that change. Can we simply merge this PR into main? I've been using this code to develop another integration test, and it's rock solid. And since the current integration tests are broken, why not retire that code now, so we can move cleanly forward? Or should I add more of the existing integration tests to it first? If so, which ones? Here's a list of the existing integration testscli_test.go:TestCLIReset cli_test.go:TestCLIGenerate cli_test.go:TestCLIFirstStartup conflict_test.go:TestConflictsDefault conflict_test.go:TestConflictsInitialMerge conflict_test.go:TestConflictsIndexReset conflict_test.go:TestConflictsSameContent delay_scan_test.go:TestRescanWithDelay filetype_test.go:TestFileTypeChange filetype_test.go:TestFileTypeChangeSimpleVersioning filetype_test.go:TestFileTypeChangeStaggeredVersioning http_test.go:TestHTTPGetIndex http_test.go:TestHTTPGetIndexAuth http_test.go:TestHTTPOptions http_test.go:TestHTTPPOSTWithoutCSRF ignore_test.go:TestIgnores manypeers_test.go:TestManyPeers override_test.go:TestOverride override_test.go:TestOverrideIgnores parallel_scan_test.go:TestRescanInParallel reconnect_test.go:TestReconnectReceiverDuringTransfer reconnect_test.go:TestReconnectSenderDuringTransfer reset_test.go:TestReset scan_test.go:TestScanSubdir symlink_test.go:TestSymlinks symlink_test.go:TestSymlinksSimpleVersioning symlink_test.go:TestSymlinksStaggeredVersioning sync_test.go:TestSyncCluster # already implemented in this PR sync_test.go:TestSyncSparseFile transfer-bench_test.go:TestBenchmarkTransferManyFiles transfer-bench_test.go:TestBenchmarkTransferLargeFile1G transfer-bench_test.go:TestBenchmarkTransferLargeFile2G transfer-bench_test.go:TestBenchmarkTransferLargeFile4G transfer-bench_test.go:TestBenchmarkTransferLargeFile8G transfer-bench_test.go:TestBenchmarkTransferLargeFile16G transfer-bench_test.go:TestBenchmarkTransferLargeFile32G transfer-bench_test.go:TestBenchmarkTransferSameFiles |
Our integration tests have been broken and not run (or even compiled) for many years. This removes them and revives the concept with a simple two-device sync setup.
The reason the old tests fell into disuse is that the concept was misguided. We built complex, unmaintainable scenarios that were flaky and took a long time to run (and then failed for random reasons). We tried to have set configs on disk, so only one instance could run at a time, and those configs were sometimes changed and then changed back in tests...
The new tests I added are perhaps not enormously valuable, but demonstrate the concept of using ephemeral Syncthing instances that can run concurrently and don't interfere with each other.
All in all, I think this is an improvement, even though almost everything of the old is simply thrown away.
The (new) integration tests are now run as part of the build, on Linux, macOS and Windows.